-
Notifications
You must be signed in to change notification settings - Fork 408
[Router] Upstream Fine-Grained Parallel Router (FPT'24) #2920
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
[Router] Upstream Fine-Grained Parallel Router (FPT'24) #2920
Conversation
Upstreamed the fine-grained parallel router implementation into the VTR master. The original branch is https://github.com/verilog-to-routing/vtr-verilog-to-routing/tree/mq-parallel-router. Modified the MultiQueue (SPAA'24) implementation and integrated it into the VTR codebase.
The original FG parallel router used to use boost. VTR does not install boost by default. Moved to STL instead.
Added a partial abstract class for ConnectionRouter, derived from the pure abstract ConnectionRouterInterface. The SerialConnectionRouter and ParallelConnectionRouter classes are now derived from the ConnectionRouter class, utilizing the common class members and helper functions to reduce code duplication.
Added Doxygen-style code comments and documentation for connection routers, including the ConnectionRouter abstract class, the Parallel- ConnectionRouter concrete class, and the SerialConnectionRouter concrete class. Updated the helper messages for command-line options added for parallel connection router.
Fixed the interface issues of ConnectionRouter in NestedNetlistRouter. Fixed code formats. Fixed typo in read_options.cpp.
Updated the command-line usage for parallel connection router in both Read the Docs and read_options.cpp.
Added regression tests for parallel connection router by appending extra sets of configurations to those VTR flow regression tests previously selected by Fahri for testing coarse-grained parallel router. Removed VPR connection router test (vpr/test/test_connection_router.cpp), since it has been out-dated for a very long time and has caused lots of trouble for running VPR C++ tests locally.
Fixed a weird code formatting issue in libs/librtlnumber/src/include/ internal_bits.hpp. GitHub CI said the file failed dev/check-format.sh, however, the same script runs perfectly in my local environment. Double checked the version of clang-format, which seemed to be the same as CI. Directly copied the file from the GitHub repo to resolve this issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ueqri
These changes are looking great! I have left some comments.
You mentioned to me that the parallel router code was not 100% commented yet, so I will review that after you give me the go ahead.
The `No source in route tree` bug in ParallelConnectionRouter (since commit 875b98e) has been fixed. It turns out that putting another member variable `MultiQueueDAryHeap<HeapImplementation::arg_D> heap_` in the derived class ParallelConnectionRouter together with the existing `HeapImplementation heap_` in the base class ConnectionRouter causes the issue. The solution is to keep `heap_` only in the base class and use `ConnectionRouter<MultiQueueDAryHeap<HeapImplementation::arg_D>>` rather than `ConnectionRouter<HeapImplementation>` for deriving the parallel connection router. Please note that ParallelConnectionRouter still has some bugs (i.e., getting stuck in the MultiQueue pop). This commit is not fully working. Please do not use it for any experiments. Updated the previously incorrect command-line options for the parallel connection router in the regression tests.
Fixed the weird bug in parallel connection router as mentioned in commit f73212c. The bug occurred because two function parameters 'num_threads' and 'num_queues' have been misplaced when instantiating the MQ_IO. This took two weeks to figure out exactly. The VTR benchmark (`vtr_reg_qor_chain` task) has been tested/passed for different cases (1) 'serial mode' 1T+2Q (1 thread, 2 queues), (2) 2T+4Q, and (3) 4T+2Q. The determinism has also been verified for the VTR benchmark.
Added more explanation to the command-line options messages and code comments. Cleaned up ParallelConnectionRouter-related codebase.
Hi Vaughn, Alex, and Fahri, this PR is ready to review eventually! @vaughnbetz @AlexandreSinger @duck2 The weird bug (causing the VTR router to get stuck) has been fixed, which is detailed in 46d6c2c. I have run
Hi Fahri @duck2, please switch to the branch in this PR if needed since this one has been more extensively tested and works well. |
@ueqri That's great news! I am very happy that the bug was something simple. It happens whenever an API has two arguments of the same type. Thanks for fixing my comments, I really like using smart pointers whenever possible in VTR. I will give it another once over sometime today. |
Updated the golden results for CI tests for parallel connection router: - `vtr_reg_strong/koios_test` - `vtr_reg_strong/strong_flat_router` - `vtr_reg_strong/strong_multiclock` - `vtr_reg_strong/strong_timing`
Upstreamed the fine-grained parallel router implementation into the VTR master. The original branch is https://github.com/verilog-to-routing/vtr-verilog-to-routing/tree/mq-parallel-router.
Modified the MultiQueue (SPAA'24) implementation and integrated it into the VTR codebase.
VTR strong regression tests passed.
TODOs